Use node interface pointers in the TransformListener class #100
Conversation
In the TransformListener class, use node interfaces instead of using the node directly so that the code works with either rclcpp::Node or rclcpp_lifecycle::LifecycleNode. Retain the existing node-based interface for backwards compatibility.
|
@tfoote @mjeronimo - Kindly review. |
|
@lbegani Looks good to me. @tfoote - One unfortunate consequence of ROS2's use of the interface pointers is that there is code duplication across the various classes that use these interfaces. For example, when creating a subscription, the topics interface itself isn't enough; there are templates on top of this required to get a memory strategy, etc. See:
Perhaps these templates could be factored out somehow so that they could be used by any class that uses the topics interface (likewise with other interfaces that require addition "glue" templates). |
I'm working on something like this for another feature, see: And it being used: As a result, the I think a similar pattern could be provided for each node interface, and used in cases like this to decouple these classes from |
There was a problem hiding this comment.
There are a couple of changes I'd like to have changed. Passing the shared pointer by value and re-using the rclcpp::create_subscription free function.
I am not sure about the priority of this package, but having these changes in for the dashing API freeze would be a great thing.
| TransformListener(tf2::BufferCore & buffer, bool spin_thread = true); | ||
|
|
||
| TF2_ROS_PUBLIC | ||
| TransformListener(tf2::BufferCore & buffer, rclcpp::Node::SharedPtr nh, bool spin_thread = true); |
There was a problem hiding this comment.
this might be up for discussion, but I advocate for removing this constructor completely and thus force the usage of the node interfaces.
|
|
||
| return rclcpp::create_subscription<MessageT, CallbackT, Alloc, CallbackMessageT, SubscriptionT>( |
There was a problem hiding this comment.
why not calling this function directly?
| TransformListener(tf2::BufferCore& buffer, rclcpp::Node::SharedPtr nh, bool spin_thread = true); | ||
| TransformListener( | ||
| tf2::BufferCore & buffer, | ||
| const rclcpp::node_interfaces::NodeBaseInterface::SharedPtr & node_base, |
There was a problem hiding this comment.
don't pass a shared pointer by reference.
| /// Initialize this transform listener, subscribing, advertising services, etc. | ||
| void init(); | ||
| void initThread(); | ||
|
|
||
| template< |
There was a problem hiding this comment.
this signature should not be necessary. Including this header file should be enough
|
|
||
| TransformListener::TransformListener( | ||
| tf2::BufferCore & buffer, | ||
| const rclcpp::node_interfaces::NodeBaseInterface::SharedPtr & node_base, |
There was a problem hiding this comment.
don't pass a shared pointer by reference
| buffer_.setUsingDedicatedThread(true); | ||
| } | ||
|
|
||
| template< |
There was a problem hiding this comment.
same comment as before.
| const tf2_msgs::msg::TFMessage & msg_in = *msg; | ||
| // TODO(tfoote) find a way to get the authority | ||
| std::string authority = "Authority undetectable"; // msg_evt.getPublisherName(); // lookup the authority | ||
| for (unsigned int i = 0; i < msg_in.transforms.size(); i++) { |
There was a problem hiding this comment.
nitpick: for (auto i = 0u; ..)
|
in order to push this forward into dashing API freeze, I've opened #108 which is based on top of this PR. It's up for review |
|
Replaced by #108 |
In the TransformListener class, use node interfaces instead of using the node directly so that the code works with either rclcpp::Node or rclcpp_lifecycle::LifecycleNode. Retain the existing node-based interface for backwards compatibility.
This change fixes the issue reported here - #94